fix: prevent silent upgrade failures from locking users on old versions#428
Closed
anandgupta42 wants to merge 1 commit intomainfrom
Closed
fix: prevent silent upgrade failures from locking users on old versions#428anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
The upgrade path is the most critical code — if it breaks, users are
permanently locked out. Two fixes:
1. Replace `.catch(() => {})` in worker.ts with error logging so upgrade
failures are visible instead of silently swallowed
2. Add comprehensive tests (41 total) including:
- semver bundling smoke tests (verify import works, not externalized)
- Decision logic tests for every branch in upgrade()
- Source assertion that checkUpgrade logs errors (not empty catch)
- Installation.VERSION format validation
Keeps semver — it bundles correctly into the compiled binary (verified:
not in build.ts external list). The real issue was the silent .catch.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThe PR modifies error handling in the upgrade check utility to log failures via console error instead of silently ignoring them, and significantly expands test coverage to validate semver dependency availability and upgrade error handling behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opencode/src/cli/cmd/tui/worker.ts (1)
289-293: Consider usingLog.Default.errorfor consistency.The rest of this file uses
Log.Default.error()for error logging (lines 28–36, 251–253), but this change usesconsole.error()directly. Unless there's a specific reason (e.g., Log not being initialized at this point), consider aligning with the existing pattern for consistent observability.♻️ Suggested change
await upgrade().catch((err) => { // Never silently swallow upgrade errors — if this fails, users // get locked on old versions with no way to self-heal. - console.error("[upgrade] check failed:", String(err)) + Log.Default.error("[upgrade] check failed", { + error: err instanceof Error ? err.message : String(err), + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/cli/cmd/tui/worker.ts` around lines 289 - 293, The upgrade error handler is using console.error instead of the project's logging abstraction; update the catch for the await upgrade() call to call Log.Default.error(...) with the same message and error details (e.g., Log.Default.error("[upgrade] check failed: %s", String(err))) so logging is consistent with other uses in this file (see other Log.Default.error usages and the upgrade() call). Ensure Log is in scope/imported where upgrade() is invoked before replacing console.error.packages/opencode/test/cli/upgrade-decision.test.ts (1)
192-193: Consider using ES imports or Bun APIs for consistency.The test file uses ES module imports at the top but switches to
require("fs")andrequire("path")inline. For consistency, consider usingBun.file()or top-level imports.♻️ Alternative using Bun.file()
test("semver is NOT in the build external list", async () => { - const fs = require("fs") - const path = require("path") - const buildScript = fs.readFileSync( - path.join(import.meta.dir, "../../script/build.ts"), - "utf-8", - ) + const buildScript = await Bun.file( + import.meta.dir + "/../../script/build.ts" + ).text()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/cli/upgrade-decision.test.ts` around lines 192 - 193, Replace the inline CommonJS calls const fs = require("fs") and const path = require("path") with consistent ES-style imports or Bun APIs used elsewhere in the file; e.g., add top-level imports (import fs from "fs"; import path from "path") or use Bun.file()/Bun.stat() where file I/O is needed so the test matches the module style and runtime expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/cli/upgrade-decision.test.ts`:
- Around line 189-204: The test "semver is NOT in the build external list" can
silently succeed when the regex fails; update it to explicitly fail when no
external array is found by asserting that externalMatch is truthy (or throwing a
clear error) before inspecting externalMatch[1]; locate the variables
buildScript and externalMatch in that test and add an explicit expectation like
expect(externalMatch).toBeTruthy() with a message such as "external array not
found in build.ts" so the test fails loudly if build.ts format changes.
---
Nitpick comments:
In `@packages/opencode/src/cli/cmd/tui/worker.ts`:
- Around line 289-293: The upgrade error handler is using console.error instead
of the project's logging abstraction; update the catch for the await upgrade()
call to call Log.Default.error(...) with the same message and error details
(e.g., Log.Default.error("[upgrade] check failed: %s", String(err))) so logging
is consistent with other uses in this file (see other Log.Default.error usages
and the upgrade() call). Ensure Log is in scope/imported where upgrade() is
invoked before replacing console.error.
In `@packages/opencode/test/cli/upgrade-decision.test.ts`:
- Around line 192-193: Replace the inline CommonJS calls const fs =
require("fs") and const path = require("path") with consistent ES-style imports
or Bun APIs used elsewhere in the file; e.g., add top-level imports (import fs
from "fs"; import path from "path") or use Bun.file()/Bun.stat() where file I/O
is needed so the test matches the module style and runtime expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12ae05e0-ce08-4b27-8a92-c20e67fde996
📒 Files selected for processing (2)
packages/opencode/src/cli/cmd/tui/worker.tspackages/opencode/test/cli/upgrade-decision.test.ts
Comment on lines
+189
to
+204
| test("semver is NOT in the build external list", () => { | ||
| // If semver is externalized, it won't be bundled in the compiled binary. | ||
| // This test reads build.ts to verify it's not listed. | ||
| const fs = require("fs") | ||
| const path = require("path") | ||
| const buildScript = fs.readFileSync( | ||
| path.join(import.meta.dir, "../../script/build.ts"), | ||
| "utf-8", | ||
| ) | ||
| // Extract the external array | ||
| const externalMatch = buildScript.match(/external:\s*\[([\s\S]*?)\]/) | ||
| if (externalMatch) { | ||
| expect(externalMatch[1]).not.toContain('"semver"') | ||
| expect(externalMatch[1]).not.toContain("'semver'") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Test silently passes if build script format changes.
If the regex doesn't find an external: array (e.g., because build.ts is refactored), externalMatch is null and no assertions run—giving a false sense of security. Consider making the expectation explicit.
🛡️ Suggested fix
const externalMatch = buildScript.match(/external:\s*\[([\s\S]*?)\]/)
- if (externalMatch) {
- expect(externalMatch[1]).not.toContain('"semver"')
- expect(externalMatch[1]).not.toContain("'semver'")
- }
+ // If there's an external array, ensure semver is not in it.
+ // If no external array exists, bundler bundles everything by default.
+ if (externalMatch) {
+ expect(externalMatch[1]).not.toContain('"semver"')
+ expect(externalMatch[1]).not.toContain("'semver'")
+ } else {
+ // Explicitly acknowledge no external array was found (semver bundled by default)
+ expect(buildScript).toContain("Bun.build") // sanity check we're reading the right file
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("semver is NOT in the build external list", () => { | |
| // If semver is externalized, it won't be bundled in the compiled binary. | |
| // This test reads build.ts to verify it's not listed. | |
| const fs = require("fs") | |
| const path = require("path") | |
| const buildScript = fs.readFileSync( | |
| path.join(import.meta.dir, "../../script/build.ts"), | |
| "utf-8", | |
| ) | |
| // Extract the external array | |
| const externalMatch = buildScript.match(/external:\s*\[([\s\S]*?)\]/) | |
| if (externalMatch) { | |
| expect(externalMatch[1]).not.toContain('"semver"') | |
| expect(externalMatch[1]).not.toContain("'semver'") | |
| } | |
| }) | |
| test("semver is NOT in the build external list", () => { | |
| // If semver is externalized, it won't be bundled in the compiled binary. | |
| // This test reads build.ts to verify it's not listed. | |
| const fs = require("fs") | |
| const path = require("path") | |
| const buildScript = fs.readFileSync( | |
| path.join(import.meta.dir, "../../script/build.ts"), | |
| "utf-8", | |
| ) | |
| // Extract the external array | |
| const externalMatch = buildScript.match(/external:\s*\[([\s\S]*?)\]/) | |
| // If there's an external array, ensure semver is not in it. | |
| // If no external array exists, bundler bundles everything by default. | |
| if (externalMatch) { | |
| expect(externalMatch[1]).not.toContain('"semver"') | |
| expect(externalMatch[1]).not.toContain("'semver'") | |
| } else { | |
| // Explicitly acknowledge no external array was found (semver bundled by default) | |
| expect(buildScript).toContain("Bun.build") // sanity check we're reading the right file | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/cli/upgrade-decision.test.ts` around lines 189 - 204,
The test "semver is NOT in the build external list" can silently succeed when
the regex fails; update it to explicitly fail when no external array is found by
asserting that externalMatch is truthy (or throwing a clear error) before
inspecting externalMatch[1]; locate the variables buildScript and externalMatch
in that test and add an explicit expectation like
expect(externalMatch).toBeTruthy() with a message such as "external array not
found in build.ts" so the test fails loudly if build.ts format changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The upgrade path is the most critical code — if it breaks, users are permanently locked out. Two fixes:
.catch(() => {})in worker.ts with error logging so upgrade failures are visible instead of silently swallowedKeeps semver — it bundles correctly into the compiled binary (verified: not in build.ts external list). The real issue was the silent .catch.
Summary
What changed and why?
Test Plan
How was this tested?
Checklist
Summary by CodeRabbit
Bug Fixes
Tests